Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

after_save_html and after_save_screenshot callbacks #171

Merged
merged 1 commit into from
Sep 21, 2016
Merged

after_save_html and after_save_screenshot callbacks #171

merged 1 commit into from
Sep 21, 2016
+119 −0

Conversation

glaszig
Copy link
Contributor

@glaszig glaszig commented Apr 24, 2016 •
edited
Loading

this adds two callbacks to the saver, after_save_html and after_save_screenshot which will allow for things like this:

# email screenshots somewhere upon failure
Capybara::Screenshot.after_save_screenshot do |path|
  mail = Mail.new do
    delivery_method :sendmail
    from     '[email protected]'
    to       '[email protected]'
    subject  'Capybara Screenshot'
    add_file File.read path
  end
  mail.delivery_method :sendmail
  mail.deliver
end

this also should make integrating s3, dropbox, backblaze, gcs and other things easier.

the callbacks mixin can also be used to define more callbacks later:

class Foo
  include Callbacks
  define_callback :run_shit

  def bar
    # ...
    run_callbacks :run_shit, *args
  end
end

Foo.run_shit do |*args|
  puts args
end

i would have liked to use Module::prepend but 1.9.3 compatibility requires alias method chaining.

@mattheworiordan
Copy link
Owner

I really like this @glaszig, it's a nice bit of work.

I have one question though that I would like to discuss before merging this in. Why did you patch the Capybara::Screenshot::Saver class with https://github.com/glaszig/capybara-screenshot/blob/callbacks/lib/capybara-screenshot/callbacks/integrations/saver.rb instead of just modifying the Saver class and thus removing the amount of "magic" in the system?

My concern is that anyone reading the Saver class directly would now not expect that another class included for callbacks is aliasing the defined methods. I personally would like to avoid these types of gotchas.

WDYT?

@glaszig
Copy link
Contributor Author

glaszig commented Apr 26, 2016

this was just me wanting to touch as little existing code as possible. but it would have made it easier to extract into a gem in case you didn't want it as core functionality.

i can, of course, integrate it right into the saver class itself and remove that integration cruft.

@mattheworiordan
Copy link
Owner

i can, of course, integrate it right into the saver class itself and remove that integration cruft.

Yes please, I would far prefer that

@glaszig
Copy link
Contributor Author

glaszig commented May 23, 2016

done. lgtm.

@glaszig
Copy link
Contributor Author

glaszig commented Sep 15, 2016

bump.

@mattheworiordan
Copy link
Owner

Nice, thanks @glaszig!

@mattheworiordan mattheworiordan merged commit 739a5cf into mattheworiordan:master Sep 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@glaszig @mattheworiordan @arielsilvestri